Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(exporter-metrics-otlp-http): add option to set the exporter aggregation preference #4409

Conversation

AkselAllas
Copy link
Contributor

Context is here

Should probably get some tests.

@AkselAllas AkselAllas requested a review from a team January 10, 2024 15:18
Copy link

linux-foundation-easycla bot commented Jan 10, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@AkselAllas
Copy link
Contributor Author

@pichlermarc Can you review this and advise on tests perhaps?

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tackling this @AkselAllas
In it's current state, I think this might not yet work, fortunately it'll be easy to fix (see comment) 🙂

For tests, just having a few tests in this file, which tests if the aggregationSelector calls the selector supplied to the constructor, and returns it's results should be sufficient. A further test to check that it returns the default aggregation preference when nothing is supplied would also be nice. 🙂

Comment on lines 120 to 122
if (config?.aggregationSelector) {
this._otlpExporter.aggregationSelector = config.aggregationSelector
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sets the selector to the internal exporter, so It won't be used.
We'd have to do something like here where we implement selectAggregation and then call the selector from the config.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep that's in the PeriodicExportingMetricReader accessing any PushMetricExporter that's passed in (which, in this case is an instance of the class in this file). However this code here sets it on_otlpExporter (which is just a field of this class that is used internally), so it's never picked up in the PeriodicExportingMetricReader

Copy link
Contributor Author

@AkselAllas AkselAllas Jan 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I should have had that comment as draft :) I was already fixing that part of the missing code.

@AkselAllas AkselAllas force-pushed the add-aggregation-selector-to-otlp-metric-exporter branch from 4819ba2 to e0f076d Compare January 17, 2024 13:40
@AkselAllas
Copy link
Contributor Author

Running npm run test doesn't work locally for me 🤔

Error: Cannot find module '../version'

Any ideas?

Copy link

codecov bot commented Jan 17, 2024

Codecov Report

Merging #4409 (6072a65) into main (828f2ed) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4409   +/-   ##
=======================================
  Coverage   92.23%   92.23%           
=======================================
  Files         336      336           
  Lines        9525     9533    +8     
  Branches     2020     2022    +2     
=======================================
+ Hits         8785     8793    +8     
  Misses        740      740           
Files Coverage Δ
...er-metrics-otlp-http/src/OTLPMetricExporterBase.ts 94.11% <100.00%> (+0.78%) ⬆️
...metrics-otlp-http/src/OTLPMetricExporterOptions.ts 100.00% <ø> (ø)

@AkselAllas AkselAllas force-pushed the add-aggregation-selector-to-otlp-metric-exporter branch 2 times, most recently from 0fad7d0 to 396b6d1 Compare January 17, 2024 18:04
@trentm
Copy link
Contributor

trentm commented Jan 17, 2024

Error: Cannot find module '../version'

That file is usually generated as part of npm run compile (in the package subdir or at the top-level of the repo clone). To just update/generate the version.ts file: npm run version:update.

@AkselAllas
Copy link
Contributor Author

Again local tests failed :(
Seemingly no actionable error though

image

@AkselAllas AkselAllas force-pushed the add-aggregation-selector-to-otlp-metric-exporter branch from 86df50a to b80e84d Compare January 18, 2024 10:53
@AkselAllas
Copy link
Contributor Author

AkselAllas commented Jan 22, 2024

@pichlermarc Is there anything to improve here? Or is there something we are waiting after?

If merged, any idea when a release could be expected? Along with next release of @opentelemetry/instrumentation I assume?

@pichlermarc pichlermarc changed the title feat(add-aggregation-selector-option-to-otlp-metric-exporter) feat(exporter-metrics-otlp-http): add option to set the exporter aggregation preference Jan 25, 2024
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more comments, then this should be good to merge. 🙂
You can expect this to release within the next two weeks (which is the rough release-schedule we try to keep).


it('aggregationSelector returns the default aggregation preference when nothing is supplied', () => {
const exporter = new OTLPMetricExporter({
temporalityPreference: AggregationTemporalityPreference.CUMULATIVE,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove it since it does not affect the test in any way and we don't assert on it. 🙂

Suggested change
temporalityPreference: AggregationTemporalityPreference.CUMULATIVE,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


export interface OTLPMetricExporterOptions extends OTLPExporterConfigBase {
temporalityPreference?:
| AggregationTemporalityPreference
| AggregationTemporality;
aggregationSelector?: AggregationSelector;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think preference may be the better name as it can be overwritten by a view.

Suggested change
aggregationSelector?: AggregationSelector;
aggregationPreference?: AggregationSelector;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -8,6 +8,8 @@ All notable changes to experimental packages in this project will be documented

### :rocket: (Enhancement)

* feat(exporter-metrics-otlp-http) [#4409](https://github.com/open-telemetry/opentelemetry-js/pull/4409) @AkselAllas
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* feat(exporter-metrics-otlp-http) [#4409](https://github.com/open-telemetry/opentelemetry-js/pull/4409) @AkselAllas
* feat(exporter-metrics-otlp-http): add option to set the exporter aggregation preference [#4409](https://github.com/open-telemetry/opentelemetry-js/pull/4409) @AkselAllas

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@AkselAllas AkselAllas force-pushed the add-aggregation-selector-to-otlp-metric-exporter branch from a2246ed to 590392d Compare January 26, 2024 11:28
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for taking care of this 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants